Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Humble] Use Fast-DDS Waitsets instead of listeners (backport #619) #633

Merged
merged 1 commit into from
Sep 7, 2022
Merged

[Humble] Use Fast-DDS Waitsets instead of listeners (backport #619) #633

merged 1 commit into from
Sep 7, 2022

Conversation

MiguelCompany
Copy link
Collaborator

Signed-off-by: Miguel Company MiguelCompany@eprosima.com

@richiware @MiguelCompany
Use Fast-DDS Waitsets instead of listeners (backport ros2#619)
cdc9816 
Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>
@MiguelCompany MiguelCompany mentioned this pull request Aug 30, 2022
Merged
4 tasks
@MiguelCompany MiguelCompany changed the title Use Fast-DDS Waitsets instead of listeners (backport #619) [Humble] Use Fast-DDS Waitsets instead of listeners (backport #619) Aug 30, 2022
@fujitatomoya
Copy link
Collaborator

CI(Full build and test with rmw_fastrtps only): https://gist.githubusercontent.com/fujitatomoya/635b829c55226943225a7669b61a0cfd/raw/c860c9ae967778906b6ad848cb82d783fae8e5ea/ros2.repos

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

why do we need to bakcport this?
It's a huge change, I don't think it needs to be backported

@MiguelCompany
Copy link
Collaborator Author

why do we need to bakcport this? It's a huge change, I don't think it needs to be backported

The change is big, yes. But it fixes several issues on the default RMW, like the ones listed here and here.

According to our testing, it also improves the overall behavior regarding discovery and services.

@alsora
Copy link
Contributor

alsora commented Sep 1, 2022

+1 on merging this.
If we don't backport it, a lot of users will likely have to build from sources and cherry-pick it as this fixes critical bugs.

@fujitatomoya
Copy link
Collaborator

a couple of CI failure are unrelated, ready to merge with 2nd review.

@ivanpauno ivanpauno added the enhancement New feature or request label Sep 1, 2022
@jefferyyjhsu jefferyyjhsu mentioned this pull request Sep 1, 2022
Merged
alsora added a commit to irobot-ros/rmw_fastrtps that referenced this pull request Sep 2, 2022
@alsora
Merge pull request #29 from jefferyyjhsu/irobot/humble
b5d5c93 
Update irobot/humble to include ros2#633
audrow
audrow approved these changes Sep 6, 2022
View reviewed changes
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a large change. I don't think it breaks API for any of the rmw_ functions, so if I understand correctly, it looks okay to be backported into Humble.

@audrow
Copy link
Member

audrow commented Sep 6, 2022 •
edited
Loading

Here's another run of CI, just to make sure.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@audrow
Copy link
Member

audrow commented Sep 7, 2022

The Windows gets the same CI warning as in @fujitatomoya's previous run, so I'm going to merge.

@audrow audrow merged commit fe73970 into ros2:humble Sep 7, 2022
@MiguelCompany
Copy link
Collaborator Author

@audrow When will this be available on binaries?

@MiguelCompany MiguelCompany deleted the backport/619 branch September 13, 2022 09:32
@benjinne benjinne mentioned this pull request Sep 26, 2022
Closed
@doisyg doisyg mentioned this pull request Oct 19, 2022
Closed
@clalancette clalancette mentioned this pull request Oct 25, 2022
Closed
@MiguelCompany
Copy link
Collaborator Author

@audrow When will this be available on binaries?

@audrow @clalancette could a release of rwm_fastrps_cpp be done any time soon?

@hplatou hplatou mentioned this pull request Dec 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audrow audrow audrow approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@clalancette clalancette Awaiting requested review from clalancette

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@MiguelCompany @fujitatomoya @ivanpauno @alsora @audrow @richiware